-
Notifications
You must be signed in to change notification settings - Fork 757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] "Not enough comments" #607 patch #646
base: uinverse
Are you sure you want to change the base?
Conversation
+ additions to impl.Main
System.out.flush(); | ||
// create a string variable called platformDependentExpectedResult which replaces all of the newlines in the constant parameter s of type string with the system line seperator, fetched by passing "line.seperator" to the method getProperty on class System | ||
String platformDependentExpectedResult = s.replaceAll("\\n", System.getProperty("line.separator")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes for anyone in the future; this line of code is currently not Enterprise Quality™ due to the fact that it does not import com.seriouscompany.business.java.fizzbuzz.packagenamingpackage.impl.Constants
and use the constant LINE_SEPERATOR
. To make this Enterprise Quality™, is it possible for somebody to start a pull request to change this singular line of code and make it follow industry standard by adding as many imports as possible to greatly optimize the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will increase team efficiency
Please make sure this doesn't break compatibility with OS/2 and Windows NT 4.0, which are both used by our company. |
Please schedule a meeting with all of us so that we can align on whether this direction is the right way to go. Also, please deliver us a high level overview of the trade-offs and benefits you considered before deciding to go in this direction with these code changes. edit: Make sure to include latency considerations, as well, because we need to make sure we do this properly before we make any commitments. |
According to the company style guide, all single-line comments must start with a capital letter for maximal readability. Additionally if the comment is a complete sentence it should end with a period. Please update your PR accordingly. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣
import com.seriouscompany.business.java.fizzbuzz.packagenamingpackage.interfaces.strategies.FizzBuzzSolutionStrategy; | ||
|
||
/** | ||
* Standard FizzBuzz | ||
*/ | ||
@Service | ||
public class StandardFizzBuzz implements FizzBuzz { | ||
|
||
// create a private contstant called _fizzBuzzSolutionStrategyFactory of type FizzBuzzSolutionStrategyFactory imported from com.seriouscompany.business.java.fizzbuzz.packagenamingpackage.interfaces.factories.FizzBuzzSolutionStrategyFactory on line 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, should read "constant"
I like the idea but we should think about accessibility too. We can't just "add" comments with no regard to the languages the reader may or may not speak, can we add a translation feature to this? I've already invited everyone involved in here to bi-daily meetings of an hour, there we can discuss our plan of action, track the progress, which languages to support and maybe even determine what a language even is in the first place. |
Could you please see if this is already proposed in #549 ? |
Add comments.
This will close #607.
Original conversation was at #608; moved due to internal technical difficulties.